test(ledger): cover a subaccount-qualified ICRC-2 self-spend - #11139
Open
mbjorkqvist wants to merge 3 commits into
Open
test(ledger): cover a subaccount-qualified ICRC-2 self-spend#11139mbjorkqvist wants to merge 3 commits into
mbjorkqvist wants to merge 3 commits into
Conversation
A spend needs no allowance only when the spender is the account it spends from, and the ledger decides that on the whole account rather than the owner alone. The suite covered this only for the default subaccount, so neither half of the subaccount case was pinned: that naming the account's own subaccount succeeds, and that naming none fails for want of an allowance even though the owner matches. Callers rely on the rule — the ckETH minter burns from its own fee subaccount — so a regression should surface here rather than in a consumer's integration test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a shared state-machine regression test to ensure ICRC-2 transfer_from correctly treats “self-spend without allowance” as equality on the full account (owner + subaccount), not just on the owner principal. This protects callers that burn/spend from their own non-default subaccounts (e.g., fee subaccounts) by catching regressions in the ledger’s own test suite.
Changes:
- Introduce
test_transfer_from_self_subaccountto verify (a) missing spender subaccount fails withInsufficientAllowanceand writes no block, and (b) specifying the matching subaccount succeeds without allowance. - Wire the new shared test into both the ICRC-1 ledger test suite and the ICP ledger test suite.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| rs/ledger_suite/tests/sm-tests/src/lib.rs | Adds the shared state-machine test covering subaccount-qualified ICRC-2 self-spend behavior and block index expectations. |
| rs/ledger_suite/icrc1/ledger/tests/tests.rs | Registers the new shared test in the ICRC-1 ledger test harness. |
| rs/ledger_suite/icp/ledger/tests/tests.rs | Registers the new shared test in the ICP ledger test harness. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…dgers Burning is how an account holding tokens under a subaccount gives them up, and it is a self-spend when the spender names that same subaccount, so the ledger charges no fee and reduces the supply. Kept separate from the allowance test and wired into the ICRC ledgers only: the ICP ledger checks an allowance for a burn even when the spender is the account itself, although it exempts that case when consuming one, so the same call fails there. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The two ledgers disagree, so the shared test returns the outcome and each caller states what its own ledger owes: an ICRC ledger accepts the burn, the ICP ledger rejects it for want of an allowance it would never have consumed. Characterising the ICP behaviour beats omitting it. The rejected path is now asserted to leave balance and supply untouched, and closing the gap in Operation::Burn has to flip this assertion rather than pass unnoticed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
gregorydemay
approved these changes
Aug 14, 2026
gregorydemay
left a comment
gregorydemay
left a comment
Contributor
There was a problem hiding this comment.
Thanks @mbjorkqvist !
This was referenced Aug 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
DEFI-2978
Why
A
transfer_fromneeds no allowance only when the spender is the account it spends from, and the ledger decides that on the whole account, not the owner alone. The state machine suite covered this for the default subaccount only (test_transfer_from_self, whose args hardcodespender_subaccount: None), so nothing pinned either half of the subaccount case: that naming the account's own subaccount succeeds, and that naming none fails for want of an allowance even though the owner matches.Callers depend on the rule — the ckETH minter burns from its own fee subaccount — so a regression in it should surface in the ledger's own suite rather than in a consumer's integration test.
What
One generic test in the shared suite, wired into the ICP and ICRC ledgers the same way
test_transfer_from_selfis, so both get it. The successful spend is asserted to land at block index 1, which also shows the rejected attempt wrote no block.Verified against
ledger_test,ledger_test_u256and the ICPledger_canister_test.